Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support VSCode Insiders build alongside Stable #3441

Merged
merged 9 commits into from Dec 1, 2017
Merged

support VSCode Insiders build alongside Stable #3441

merged 9 commits into from Dec 1, 2017

Conversation

say25
Copy link
Member

@say25 say25 commented Nov 28, 2017 •
edited

  • All Platforms

Addresses #3011 @shiftkey's comment

@msathieu maybe? I don't think we can have multiple enum values mapped to a single string (so we can separate the logic for ExternalEditor.VisualStudioCodeInsiders but make it appear in the UI as 'Visual Studio Code') so maybe we'll need to call it 'Visual Studio Code (Insiders)'.

Its pretty typical for those with VS Code Insiders installed to also have VS Code (vanilla) installed side by side. This allows each to be selected.

@shiftkey
Copy link
Member

@say25 I think we stumbled upon the linting issue you're facing in #3449 - try merging master into this branch and see if yarn lint:fix works again...

@say25
Copy link
Member Author

say25 commented Nov 28, 2017

will do tonight

@say25
Copy link
Member Author

say25 commented Nov 29, 2017

@shiftkey It seems that running yarn eslint:fix locally doesn't do make any further changes to the failing files yet still errors on the build agents :/

@shiftkey
Copy link
Member

@say25 you might have to refresh your packages by running yarn again before trying to run yarn lint or yarn lint:fix.

@say25
Copy link
Member Author

say25 commented Nov 29, 2017

Seems like you were right @shiftkey I'm a yarn noob

shiftkey
shiftkey suggested changes Nov 30, 2017
View reviewed changes
Copy link
Member

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just that one difference for Windows. I'm not sure why macOS is the exception here 🤷‍♂️

app/src/lib/editors/win32.ts Outdated
@@ -83,6 +92,7 @@ function getExecutableShim(
case ExternalEditor.Atom:
return Path.join(installLocation, 'bin', 'atom.cmd')
case ExternalEditor.VisualStudioCode:
case ExternalEditor.VisualStudioCodeInsiders:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@shiftkey shiftkey self-assigned this Nov 30, 2017
@shiftkey shiftkey changed the title Support Side By Side Install support VSCode Insiders build alongside Stable Nov 30, 2017
shiftkey
shiftkey previously approved these changes Dec 1, 2017
View reviewed changes
@shiftkey
Copy link
Member

shiftkey commented Dec 1, 2017

@say25 just need to fix the conflict from merging #3467 and this should be good to merge.

@shiftkey
Copy link
Member

shiftkey commented Dec 1, 2017

I've just pushed a lint fix for that. I'll drop myself off the review and give someone else a chance to give this a final 👍 .

@shiftkey shiftkey removed their assignment Dec 1, 2017
@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 1, 2017
@say25
Copy link
Member Author

say25 commented Dec 1, 2017

@shiftkey thanks. Dont have yarn install on my windows box :/ probably should do that now.

@shiftkey
Copy link
Member

shiftkey commented Dec 1, 2017

@say25 yeah, we've switched over to using it for lockfile support (NPM@5 has some limitations that affect cross-platform libraries that remain a blocker)

@joshaber joshaber self-assigned this Dec 1, 2017
joshaber
joshaber approved these changes Dec 1, 2017
View reviewed changes
Copy link
Contributor

@joshaber joshaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨

@joshaber joshaber merged commit 5eacc0d into desktop:master Dec 1, 2017
@say25 say25 deleted the vs-code-insiders-side-by-side branch December 1, 2017 15:43
@shiftkey shiftkey mentioned this pull request Dec 3, 2017
Closed
Vovka19
Vovka19 reviewed Dec 9, 2017
View reviewed changes
Copy link

@Vovka19 Vovka19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@steveward steveward mentioned this pull request Aug 10, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vovka19 Vovka19 Vovka19 left review comments

@joshaber joshaber joshaber approved these changes

@shiftkey shiftkey shiftkey left review comments

Assignees

@joshaber joshaber

Labels
ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@say25 @shiftkey @joshaber @Vovka19